Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat introduced icp nonces for context contract #1031

Open
wants to merge 28 commits into
base: feat--add-near-calls-nonce
Choose a base branch
from

Conversation

alenmestrov
Copy link
Member

Introduced nonce logic for mutate requests in icp context contract.

Optimized context contract for less spending.
Optimized proxy contract for cross-contract calls that include deposit

@alenmestrov alenmestrov self-assigned this Dec 20, 2024
Copy link
Member

@miraclx miraclx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can deduplicate the nonce increment, check the near impl

contracts/icp/context-config/src/mutate.rs Outdated Show resolved Hide resolved
contracts/icp/context-config/src/lib.rs Show resolved Hide resolved
contracts/icp/context-config/src/mutate.rs Outdated Show resolved Hide resolved
contracts/icp/context-config/src/mutate.rs Outdated Show resolved Hide resolved
contracts/icp/context-config/src/mutate.rs Outdated Show resolved Hide resolved
@alenmestrov alenmestrov requested a review from miraclx December 20, 2024 16:43
@alenmestrov alenmestrov requested a review from miraclx December 20, 2024 17:22
Comment on lines +141 to +145
configs
.contexts
.get(&context_id)
.and_then(|context| context.member_nonces.get(&member_id))
.copied()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency, we should probably panic if the context does not exist

same as we have it above

Copy link
Member

@miraclx miraclx Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but should we be panicking in ICP at all? how do they handle that? since the state is in-memory, do we lose data?

@miraclx
Copy link
Member

miraclx commented Dec 20, 2024

And.. migrations

though there's no active deployment, so I guess we can get away with it

@alenmestrov alenmestrov requested a review from miraclx December 23, 2024 19:38

// Execute the transfer with proper type annotations
let transfer_result: Result<(Result<u64, TransferError>,), _> =
ic_cdk::call(ledger_id, "transfer", (transfer_args,)).await;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icrc2's transfer_from? or does this work too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants